Merge changes from 'dev' branch #389

gets0ul · 2019-10-16T12:13:12Z

As the title.

maxceem

As I see quite many files that have been changed in dev are not updated in v5-upgrade branch in this PR. So changed which has been in done in dev branch in the next files are missing:

.circleci/config.yml
local/seed/seedMetadata.js
local/seed/seedProjects.js
src/events/busApi.js
src/events/index.js
src/events/projectMembers/index.js
src/models/form.js
src/models/projectMemberInvite.js
src/models/projectTemplate.js
src/routes/admin/project-index-create.js
src/routes/milestones/delete.spec.js
src/routes/milestones/get.spec.js
src/routes/phaseProducts/update.js
src/routes/productCategories/create.js
src/routes/productTemplates/list.js
src/routes/productTemplates/list.spec.js
src/routes/productTemplates/upgrade.spec.js
src/routes/projectMemberInvites/update.spec.js
src/routes/projectTemplates/list.js
src/routes/projectTemplates/list.spec.js
src/routes/projectTypes/create.js
src/routes/projectUpgrade/create.js
src/routes/projectUpgrade/create.spec.js
src/routes/timelines/create.js
src/routes/timelines/create.spec.js
src/routes/timelines/delete.spec.js
src/routes/timelines/get.spec.js
src/routes/timelines/update.spec.js
src/tests/serviceMocks.js
src/tests/util.js
src/util.spec.js

To verify, you find all these files in the changes which have been done to dev: https://github.com/topcoder-platform/tc-project-service/compare/84ce3c5f0d484a80b9f985c711a1531da22609a8..1cee66ef658d285a2b87fbfca2aef892a61ef3c0.

For example .circleci/config.yml.
It has been changed in dev:

But if we check this file after merging (this PR) these changes are not here:

Same for other files in the list.

I didn't check other files yet, as I think all the files have to be merged first.

Also, I have several unit tests failing. Do they work for you? I know sometimes there are random tests failed and sometimes succeeded. But so far I run several times and these test always fail:

gets0ul · 2019-10-17T06:26:45Z

@maxceem
Thanks for the feedback.
The tests are working for me. I checked it by running them one by one as running it all in one-go takes a very long time and hard to track the log.

And for the files differences, I'll check again.

gets0ul · 2019-10-17T17:11:15Z

PR is updated.

maxceem

Thanks for the updated @gets0ul.

Most files have been updated. Though there are still several files left which look like have to be updated too:

package.json
src/routes/productCategories/create.js
src/routes/productTemplates/upgrade.spec.js
src/routes/projectMemberInvites/update.spec.js
 src/routes/projectTypes/create.js 
src/routes/projectUpgrade/create.js 
src/routes/projectUpgrade/create.spec.js

For example package.json has the next changes in dev:

But after merging, I don't see these changes:

Meanwhile, as most files have been updated. Now I'll start checking if updated files have all the changes merged from dev into v5-upgrade.

maxceem · 2019-10-18T10:05:37Z

@gets0ul I've reviewed 86 of 169 files in PR. Mostly it's good, but still, have some things to fix listed below. I will check the rest of the files tomorrow morning.

Just FYI it's quite easy to check the changes if open two windows next to each other. One window with changes done in "dev": https://github.com/topcoder-platform/tc-project-service/compare/84ce3c5f0d484a80b9f985c711a1531da22609a8..1cee66ef658d285a2b87fbfca2aef892a61ef3c0
And another window with changes done in this PR https://github.com/topcoder-platform/tc-project-service/pull/389/files
Mostly, changes should be the same as we are merging changes from "dev" into "v5-upgrade".

Issues

✅ config/custom-environment-variables.json https://monosnap.com/file/8sMjYNvVtD7GiCEDttS572XYd039PE
✅ config/default.json https://monosnap.com/file/yyWr6F9578N61sjRzflVTcSWa6gtBs
✅ local/seed/seedProjects.js https://monosnap.com/file/so7Sm6qx5KUDSdSLoUxlowek9OGFWs
✅ src/models/projectPhase.js https://monosnap.com/file/teuDYq4DSJlnK9VMTJ53S1KMXO10iL
✅ src/routes/form/version/getVersion.js https://monosnap.com/file/h0JyTT1bwGAhozAv4dWv24m2xSzqh2
✅ src/routes/metadata/list.spec.js https://monosnap.com/file/S0BlFI9WgayUsjcDEdVRscuqVutguF
✅ src/routes/milestones/create.js https://monosnap.com/file/Y5CP6llU0YbYvHJz5C3thD63TYFwUs
✅ src/routes/milestones/create.spec.js
- https://monosnap.com/file/S8DDAg3mQJw2vRB6y8sybvQjUHX57G
- https://monosnap.com/file/Nu5e6V5n8HvEExWXvu717C0NKvfjCO
✅ src/routes/milestones/delete.spec.js
- https://monosnap.com/file/e7jguxNDj3BVhZRLtQX20M3k2cfTnW
- Comment. This change hasn't been done in dev, but it looks good to me https://monosnap.com/file/0wwWAeW4TQnc2GFEgB4wLNVDZoyFnD
✅ src/routes/milestones/get.spec.js https://monosnap.com/file/te4Q7X3fUxNU29A7CfVvdgiWrN67Dt
✅ src/routes/milestones/update.spec.js
- https://monosnap.com/file/NOcmSc48e01nhTuppMmcajdbnN5b4X
- In "dev" branch "completionDate" has been removed from the "body" https://monosnap.com/file/30axb3Or1OU1Q5kHgWIkNiHhIwapBY, but in this PR - no. That's why you had to update the next unit tests https://monosnap.com/file/rmoN3DdWTD1McgLVfR1VDXRDcuK5Gu but they shouldn't be updated this way. And they should work good after removing completionDate from the body.
✅src/routes/phaseProducts/create.spec.js
- why updated to different user roles? https://monosnap.com/file/TLTQWjRh9uXXSu01ZMJgtXfxMVQmmY
- there are new tests have been added https://monosnap.com/file/WexdRq4pQGbOhtFBRe0dL9jFzfZcvK
✅ src/routes/phaseProducts/delete.spec.js
- https://monosnap.com/file/I9L2hRNOjhsCmMd9kCMb9VEF7VBVin
- https://monosnap.com/file/1ic4EYsAs3CnnuEmlc9H9WJWwl5nrL
✅ src/routes/phaseProducts/update.spec.js
- https://monosnap.com/file/H9k4PzdxHG8vX48rnpK05OtHXd6tUF
- https://monosnap.com/file/spqiR6lqAd3Q7Mbm9tbwlA7RTeWnXk
- new test have been added in "dev" https://monosnap.com/file/rQ4KJl2FSHNDcPutH1b7OiVu1BWooz
✅ src/routes/phases/create.spec.js misses many updates https://monosnap.com/file/L5h3PVP9RXvPJ31qsGYjY2IHsC5adC
✅ src/routes/phases/delete.spec.js misses many updates https://monosnap.com/file/yjycYMbFQ5kMymp9EwPLpeCl0Ao057
✅ src/routes/phases/update.spec.js many updates are missed https://monosnap.com/file/UdqYeIKFrM1xlgm1js2zKAqU91rMsX
✅ src/routes/planConfig/version/getVersion.js https://monosnap.com/file/v3T1GCXisnvfUrZYyhFPcjQGbLhz2q
✅ src/routes/priceConfig/version/getVersion.js https://monosnap.com/file/yvqQ10Oo0ekMVcRcEzuWCWr6qEVsLS
✅src/routes/productTemplates/list.js https://monosnap.com/file/72hU1VNrSf13YSrJVsuzTTwYRAudRv
✅ src/routes/productTemplates/list.spec.js https://monosnap.com/file/35njGPASIOucR0SkMHGvfn9rHpGLuS

Comments

These are just comments to keep an eye on these places during testing. No fixes are needed there.

src/routes/milestones/list.spec.js https://monosnap.com/file/204B3iDEmLjhCwQGcMaf0OHL50a5Hy
src/routes/phases/list.js not in "dev" branch, but looks like a good fix for "v5" https://monosnap.com/file/4gJyOOybNIcpnAftkhVV73NiOejgqo
src/routes/phases/list.spec.js this change hasn't been done in "dev" but it looks good. Will check it more when running unit tests https://monosnap.com/file/CTg0D1RJ8e4fMpCcPWSAwpWL23QwG3
src/routes/phaseProducts/list.spec.js this change hasn't been done in "dev" but it looks good. Will check it more when running unit tests https://monosnap.com/file/thY8qpHvcrBPATMVaLvDKiSfel4dx3
src/routes/planConfig/version/create.spec.js This change is not in "dev" but it looks good. Will check it more when running unit tests https://monosnap.com/file/qx8ugnZURT8SlyZRQf7hFnVV74oAwV
src/routes/planConfig/version/delete.spec.js same like above https://monosnap.com/file/hLSEnCJgY5tn8L8AbgF7zqx6WrhZSI
src/routes/planConfig/version/get.spec.js same like above
https://monosnap.com/file/bIhSlgMcK4xafBd4raCZOJziRtRSiw
src/routes/planConfig/version/update.spec.js same like above https://monosnap.com/file/ovPIOCu2dAYcC1h2DzqkS6Wo9RqTWe

maxceem · 2019-10-19T05:17:08Z

Thanks for the quick update @gets0ul!

I see you've updated package.json. But other files which have been listed above #389 (review) haven't been updated. Is there any reason to not update them?

✅ src/routes/productCategories/create.js
✅ src/routes/productTemplates/upgrade.spec.js
✅ src/routes/projectMemberInvites/update.spec.js
✅ src/routes/projectTypes/create.js
✅ src/routes/projectUpgrade/create.js
✅ src/routes/projectUpgrade/create.spec.js

This was regarding not updated files. I continue checking updated files...

maxceem · 2019-10-19T06:31:01Z

Issues, part 1

Most of the issues from the first part have been fixed. Only two files from them have some mismatches.

✅12.)src/routes/phaseProducts/create.spec.js https://monosnap.com/file/PUmVUi6zBbQKaroJjP42kpevg36Rw3
✅(16.) src/routes/phases/update.spec.js
- https://monosnap.com/file/BGd3sLJ4HaQuBSMa2UnSOHMRrZYJDC
- I see you've moved many tests from "dev" with "BUS_API_EVENT.....". But in the "dev" branch only one test has been added 'should NOT send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when status updated (active)' https://monosnap.com/file/p8NnsVxfAI9J134RuFYl16AubOoprw. The rest of unit tests have not been added in "dev" like should NOT send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when spentBudget updated,should NOT send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when progress updated, should NOT send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when details updated, should NOT send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when status updated (completed), should NOT send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when budget updated, should not send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when order updated, should not send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when order updated. I know they are in "dev" but they have been removed in "v5". During the merging of "dev" into "v5" we only have to merge changes which have been done in "dev" but not other code. Also, note, that we in "v5" we even don't have such constants as BUS_API_EVENT.PROJECT_PLAN_UPDATED, see https://github.com/topcoder-platform/tc-project-service/blob/v5-upgrade/src/constants.js#L98-L151.
  So actually in "v5" all these events have been removed, so even the one unit test which have been added in "dev" https://monosnap.com/file/p8NnsVxfAI9J134RuFYl16AubOoprw also should not be merged, as the constant for this event have been removed in "v5".
  Sum up:
- We should move only changes which have been done in "dev". If something has been removed in "v5" it should stay removed.
- Many events have been removed in "v5". So if you see that some code have been updated or added in "dev" regarding "BUS_API_EVENT" you may check if the constant is still there https://github.com/topcoder-platform/tc-project-service/blob/v5-upgrade/src/constants.js#L98-L151, then this code can be merged. But if it's not there - most likely it's removed and shouldn't be merged.
- Also, you've changed some new unit tests which have been added in "v5", but we shouldn't change them as are checking some new functionality in "v5"
  1. 'should send message BUS_API_EVENT.PROJECT_PHASE_UPDATED when startDate updated' renamed to 'should send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when startDate updated' and also updated the test code: https://monosnap.com/file/M2jrtERUa7DuZub2odNx14YhmEcPUJ. We should keep this new test and it should work in "v5".
  2. this test shouldn't be renamed https://monosnap.com/file/5hMUeRRuqipP3YyPkUcgDEJb7S4XP5

gets0ul · 2019-10-19T08:39:32Z

Also, you've changed some new unit tests which have been added in "v5", but we shouldn't change them as are checking some new functionality in "v5"
'should send message BUS_API_EVENT.PROJECT_PHASE_UPDATED when startDate updated' renamed to 'should send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when startDate updated' and also updated the test code: https://monosnap.com/file/M2jrtERUa7DuZub2odNx14YhmEcPUJ. We should keep this new test and it should work in "v5".
this test shouldn't be renamed https://monosnap.com/file/5hMUeRRuqipP3YyPkUcgDEJb7S4XP5

As the v5-upgrade does not have the PROJECT_PLAN_UPDATED then these tests can just be deleted, right?
Is my understanding correct?

maxceem · 2019-10-19T09:07:04Z

As the v5-upgrade does not have the PROJECT_PLAN_UPDATED then these tests can just be deleted, right?
Is my understanding correct?

Your understanding is correct. If we added some code in "dev" ragarding PROJECT_PLAN_UPDATED we shouldn't add it to "v5-upgrade", as this event has been removed from https://github.com/topcoder-platform/tc-project-service/blob/v5-upgrade/src/constants.js#L98-L151.

But regarding the quote:

Also, you've changed some new unit tests which have been added in "v5", but we shouldn't change them as are checking some new functionality in "v5"
'should send message BUS_API_EVENT.PROJECT_PHASE_UPDATED when startDate updated' renamed to 'should send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when startDate updated' and also updated the test code: https://monosnap.com/file/M2jrtERUa7DuZub2odNx14YhmEcPUJ. We should keep this new test and it should work in "v5".
this test shouldn't be renamed https://monosnap.com/file/5hMUeRRuqipP3YyPkUcgDEJb7S4XP5

The thing here is different. In v5-upgrade we had a unit test should send message BUS_API_EVENT.PROJECT_PHASE_UPDATED when startDate updated which is good. But in some reason, you renamed it to should send message BUS_API_EVENT.PROJECT_PLAN_UPDATED when startDate updated which we shouldn't do. There are no reason to rename the unit test which has been added in v5-upgrade. We only move changes from dev.

maxceem · 2019-10-19T09:41:17Z

@gets0ul In the second part much fewer mismatches. As most of the files are new there. Now I understood why the merging is going on slower than I expected. You not only merged the changes but also updated new endpoints to follow "v5" standard, so you remove the wrapper and updated unit tests. This awesome that you did that, I just initially planned that we do it step by step, so we don't make to much changes in one PR.
So I was planning to do like:

merge changes from "dev" to "v5-upgrade" without updating new code to follow "v5" standards. So we can test it first.
and only after update new code to follow "v5" standards.

As you've already updated that totally fine and even great, as I was thinking we would later need time for that.
It's just we would need to test more stuff in this PR.

Issues, part 2

✅ src/routes/projectMemberInvites/create.js https://monosnap.com/file/DfYz88lYLGn2th8xZpVCPphfpgTPo5. Also, as hint which was mentioned above.
- Many events have been removed in "v5". So if you see that some code has been updated or added in "dev" regarding "BUS_API_EVENT" you may check if the constant is still there https://github.com/topcoder-platform/tc-project-service/blob/v5-upgrade/src/constants.js#L98-L151, then this code can be merged. But if it's not there - most likely it's removed and shouldn't be merged.
✅ src/routes/projectMemberInvites/create.spec.js

✅ src/routes/projectTemplates/list.js https://monosnap.com/file/wqBGB1wKT4aAZRrbjXWq4ii6J9LDYL
- In "v5" we always get data from the ElasticSearch index. And if we cannot find it, then we try to get it from DB. This shouldn't change. If we in "dev" we updated request, then we should update the request to ES and to DB accordingly. But we shouldn't remove getting data from ES as it's done in "v5". If you don't feel comfortable to update requests to ES that's ok. Don't make these changes, but make a note in this PR so we know which places have been skipped and we will update them after merging.
✅ src/routes/projects/create.js
- https://monosnap.com/file/Zhay9sErAyetgszbLobHDCg7pUHygV
- https://monosnap.com/file/lnXRT0zf0i6SmJBEQR9wO0S1PO8Lf1
✅ src/routes/projects/create.spec.js https://monosnap.com/file/ohpGtq0Bts32csML3e7ChcolAb217t
✅ src/routes/timelines/create.js We shouldn't remove new code added in "v5". https://monosnap.com/file/GLQ8kebItZTHTpdWgPkdJKqaQNykD7
✅ src/routes/timelines/list.spec.js https://monosnap.com/file/LDA9NoGDncOzsxp4362RDmPyyGuiew

Comments, part 2

src/routes/projects/delete.spec.js Non in changed in "dev" but looks good https://monosnap.com/file/j89i2YlCiluqbxVTDbFnL0Xsc5psiU.
src/routes/projects/get.spec.js Non in changed in "dev" but looks good. Will test during unit test running. https://monosnap.com/file/F3jF8wOBsF34yQ0N0U46eccKgWv8fb
src/routes/projects/list.spec.js Non in changed in "dev" but looks good. Will test during unit test running. https://monosnap.com/file/jXoC262Y1XPzssdFFnr5pXCfeOy68Z

gets0ul · 2019-10-19T11:15:37Z

Well, you should have said that before :p
The google docs link you send to me said that unit tests should pass, so I did what I have to do here.

gets0ul · 2019-10-19T14:17:23Z

PR is updated based on feedback part 1 & 2.
Thanks.

maxceem · 2019-10-21T03:51:10Z

Thank you @gets0ul. Everything looks perfect now.

Unit tests don't pass when I run them all together, but pass when I rum them separately. We experience like that before, so I don't think it's connected with this merging. Will check it out separately.

There is just one super small mismatch, listed below.

Issues

src/routes/productCategories/create.js https://monosnap.com/file/Bb0MjW0mQTmmTcbGTUDirO6Sbc3WsX

TODO

These are some comments for me, so I don't forget. They should be implemented for now.

src/routes/projectTemplates/list.js - Update request to ES so it doesn't retrieve disabled objects. We have to consider again if we really need it. As with such logic, once we mark object as disabled we cannot get it anymore even by admins.

maxceem · 2019-10-21T03:59:15Z

@gets0ul noticed one more thing. As you've already updated new endpoints to follow "v5" format, there are a few new endpoints can be also updated, as they still use util.wrapResponse, but they shouldn't as per new "v5" standard:

✅ src/routes/projectReports/getReport.js
✅ src/routes/projectSettings/create.js
✅ src/routes/projectSettings/list.js
✅ src/routes/projectSettings/update.js

Also, a few new endpoints still use body.param while most are updated to new "v5", and don't use param inside body anymore:

✅ src/routes/projectSettings/create.js and corresponding unit tests src/routes/projectSettings/create.spec.js
✅ src/routes/projectSettings/update.js and corresponding unit tests src/routes/projectSettings/update.spec.js

…tSettings, projectReports)

gets0ul · 2019-10-21T09:56:48Z

Commit added for the remaining issues.

maxceem

Thanks @gets0ul .

All looks great now. Unit tests don't pass altogether, but pass if I run them separately. So I think that's an existent issue.

Merge changes from 'dev' branch

0589926

maxceem suggested changes Oct 17, 2019

View reviewed changes

gets0ul added 2 commits October 18, 2019 00:04

Added fixes.

f12979a

Remove duplicates.

c6abfb2

maxceem suggested changes Oct 18, 2019

View reviewed changes

More fixes based on feedback.

c5c36a5

Fixes based on feedback part 2.

0718a23

Fix remaining mismatches (productCategories, projectTemplates, projec…

8fb8b2f

…tSettings, projectReports)

maxceem approved these changes Oct 22, 2019

View reviewed changes

maxceem merged commit 36ec430 into topcoder-platform:v5-upgrade Oct 22, 2019

Merge changes from 'dev' branch #389

Merge changes from 'dev' branch #389

Uh oh!

Conversation

gets0ul commented Oct 16, 2019

Uh oh!

maxceem left a comment

Choose a reason for hiding this comment

Uh oh!

gets0ul commented Oct 17, 2019

Uh oh!

gets0ul commented Oct 17, 2019

Uh oh!

maxceem left a comment

Choose a reason for hiding this comment

Uh oh!

maxceem commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Comments

Uh oh!

maxceem commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxceem commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues, part 1

Uh oh!

gets0ul commented Oct 19, 2019

Uh oh!

maxceem commented Oct 19, 2019

Uh oh!

maxceem commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues, part 2

Comments, part 2

Uh oh!

gets0ul commented Oct 19, 2019

Uh oh!

gets0ul commented Oct 19, 2019

Uh oh!

maxceem commented Oct 21, 2019

Issues

TODO

Uh oh!

maxceem commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gets0ul commented Oct 21, 2019

Uh oh!

maxceem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxceem commented Oct 18, 2019 •

edited

Loading

maxceem commented Oct 19, 2019 •

edited

Loading

maxceem commented Oct 19, 2019 •

edited

Loading

maxceem commented Oct 19, 2019 •

edited

Loading

maxceem commented Oct 21, 2019 •

edited

Loading